Refactor extract_df.py Conversion Logic and Clean Up Global Column Names in global_scheme.py#244
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughConsolidates multiple SI→IP conversions into a single dispatch function and updates call sites. Expands ColNames with new constants. Refactors global_scheme mappings and dropdowns to use ColNames keys instead of strings. Adjusts psy-chart import and usage to the new converter. Minor coefficient change for enthalpy conversion. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as psy-chart.update_psych_chart
participant Extract as extract_df.convert_data
participant Conv as extract_df.convert_SI_to_IP
User->>UI: Select IP unit system
UI->>Extract: convert_data(df, unit_system="IP")
Extract->>Conv: convert_SI_to_IP(df, key)
Conv-->>Extract: In-place column updates (by ColNames)
Extract-->>UI: Converted df
UI-->>User: Render psychrometric chart
note right of Conv: Centralized dispatch replaces per-column helpers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pages/lib/global_column_names.py (1)
18-20: Duplicate Dew Point keys (DPT vs T_DP) will collide in name→key maps.Both map to “Dew point temperature,” causing dropdown name collisions and unpredictable selection behavior.
Recommended: standardize on ColNames.T_DP ("t_dp") and deprecate ColNames.DPT from public dropdowns (keep constant only for backward compatibility). See proposed list edits in global_scheme.py comments.
Also applies to: 28-29
pages/psy-chart.py (3)
390-393: Hover label repeats name instead of unit.Should show units after the numeric value.
- hovertemplate=mapping_dictionary[ColNames.DBT][ColNames.NAME] - + ": %{x:.2f}" - + mapping_dictionary[ColNames.DBT][ColNames.NAME], + hovertemplate=mapping_dictionary[ColNames.DBT][ColNames.NAME] + + ": %{x:.2f}" + + mapping_dictionary[ColNames.DBT][si_ip][ColNames.UNIT],
398-406: Histogram x-bins ignore unit system.Bins are fixed to SI; Fahrenheit plots will be wrong. Use unit-aware bounds.
- xbins=dict(start=-50, end=100, size=1), + xbins=dict( + start=mapping_dictionary[ColNames.DBT][si_ip][ColNames.RANGE][0], + end=mapping_dictionary[ColNames.DBT][si_ip][ColNames.RANGE][1], + size=1 if si_ip == UnitSystem.SI else 2 # ~1°C or ~2°F bins + ),
455-473: Use standardized ColNames keys in customdata and hovertemplate.Stay consistent with the PR goal; avoid raw string keys for “h” and “t_dp”.
- customdata=np.stack( - (df[ColNames.RH], df["h"], df[var], df["t_dp"]), axis=-1 - ), + customdata=np.stack( + (df[ColNames.RH], df[ColNames.EH], df[var], df[ColNames.T_DP]), axis=-1 + ), ... - + mapping_dictionary["h"][ColNames.NAME] + + mapping_dictionary[ColNames.EH][ColNames.NAME] + ": %{customdata[1]:.2f}" - + mapping_dictionary["h"][si_ip][ColNames.UNIT] + + mapping_dictionary[ColNames.EH][si_ip][ColNames.UNIT] + "<br>" - + mapping_dictionary["t_dp"][ColNames.NAME] + + mapping_dictionary[ColNames.T_DP][ColNames.NAME] + ": %{customdata[3]:.2f}" - + mapping_dictionary["t_dp"][si_ip][ColNames.UNIT] + + mapping_dictionary[ColNames.T_DP][si_ip][ColNames.UNIT]
🧹 Nitpick comments (8)
pages/lib/global_column_names.py (1)
23-29: Terminology fix: HR is “humidity ratio,” not “absolute humidity”.Labeling HR as “Absolute Humidity” conflicts with units (g/kg, lb/klb) and downstream usage (psy.psy_ta_rh returns humidity ratio). Align naming to avoid user confusion.
Apply in-place docstring/comment tweak:
- HR = "hr" # Absolute Humidity + HR = "hr" # Humidity Ratiopages/lib/global_scheme.py (4)
751-762: Rename HR display to “Humidity ratio”.Matches domain terminology and the units already configured.
ColNames.HR: { - ColNames.NAME: "Absolute humidity", + ColNames.NAME: "Humidity ratio", ColNames.COLOR: ["#ffe600", "#00c8ff", "#0000ff"], UnitSystem.SI: { ColNames.UNIT: "g water/kg dry air", ColNames.RANGE: [0, 0.03 * 1000], }, UnitSystem.IP: { ColNames.UNIT: "lb water/klb dry air", ColNames.RANGE: [0, 0.03 * 1000], }, },
856-860: Converge on a single dew point key.If you intend to keep both, give them distinct display names. Preferably, remove the ColNames.DPT mapping block or relabel it “Dew point temperature (legacy)” and ensure it’s not included in public dropdown lists.
Example relabel (if you must keep the block):
- ColNames.DPT: { - ColNames.NAME: "Dew point temperature", + ColNames.DPT: { + ColNames.NAME: "Dew point temperature (legacy)", ... },Also applies to: 775-786, 155-166
384-391: Consider mph for Wind Speed (IP).Public users generally expect mph; “fpm” is uncommon and results in large numeric ranges. If UX favors mph, convert and adjust ranges accordingly.
197-201: Nit: unit casing “Psi” → “psi”.Minor polish for consistency with common unit notation.
- ColNames.UNIT: "Psi", + ColNames.UNIT: "psi",pages/psy-chart.py (1)
426-441: Fragile check for UTCI categories by unit label.Comparing
var_unit == "Thermal stress"is brittle. Prefer explicit key checks for the four UTCI category datasets.Example:
- if var_unit == "Thermal stress": + if var in {ColNames.UTCI_SUN_WIND_CATEGORIES, ColNames.UTCI_NOSUN_WIND_CATEGORIES, + ColNames.UTCI_SUN_NOWIND_CATEGORIES, ColNames.UTCI_NOSUN_NOWIND_CATEGORIES}:pages/lib/extract_df.py (2)
446-451: These conversions are currently no-ops until the Fahrenheit case includes ADAPTIVE_ keys.*Once you add them to
convert_SI_to_IP, this block is fine. Otherwise, remove or fix per earlier comment.
402-404: Clarify mutability and supported columns in the docstring.Small doc improvement helps future maintainers.
-def convert_SI_to_IP(df: pd.DataFrame, name: str) -> None: - """Convert SI to IP based on column name.""" +def convert_SI_to_IP(df: pd.DataFrame, name: str) -> None: + """In-place SI→IP conversion for known columns (temps, pressures, irradiance, illuminance, luminance, wind, visibility, enthalpy). No-op for unknown/missing columns."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pages/lib/extract_df.py(1 hunks)pages/lib/global_column_names.py(3 hunks)pages/lib/global_scheme.py(19 hunks)pages/psy-chart.py(2 hunks)
🔇 Additional comments (6)
pages/psy-chart.py (1)
319-330: Axis range vs. data scaling: double-check HR×1000 consistency.Global ranges use mapping_dictionary[HR] already in g/kg (×1000). Local range multiplies min/max by 1000 as well. This is correct, but ensure upstream df[HR] is always kg/kg to avoid double scaling.
pages/lib/extract_df.py (5)
435-437: Confirm intended IP unit for wind speed.Factor 196.85 converts m/s→ft/min. If UI/analytics expect mph (×2.23694) or ft/s (×3.28084), this will be inconsistent.
If mph is desired:
- case ColNames.WIND_SPEED: - df[name] = df[name] * 196.85039370078738 + case ColNames.WIND_SPEED: + # m/s → mph + df[name] = df[name] * 2.2369362921
417-419: Pressure unit assumption: confirm Pa→psi for P_ATM/P_VAP/P_SAT.If
psy.psy_ta_rhreturns vapor/saturation pressure in kPa (common), factor should be ×0.145038; current value is Pa→psi.Would you confirm the units returned by
pythermalcomfort.psychrometrics.psy_ta_rhforp_satandp_vapin your version?
441-443: Good fix on enthalpy conversion constant.0.000429923 (J/kg→Btu/lb) is accurate; keeps IP enthalpy consistent.
402-402: No action needed – Python version already pinned ≥3.10
Pipfile specifiespython_version = "3.11"and.python-versionis3.10, both supporting thematchstatement introduced in Python 3.10.
432-434: Use correct cd/m²→foot-lambert conversion for ZLUMI
Replace the 0.0929 factor with ~0.291864 (1 cd/m² = 0.291864 fL).- case ColNames.ZLUMI: - df[name] = df[name] * 0.0929 + case ColNames.ZLUMI: + # cd/m² → foot-lambert + df[name] = df[name] * 0.291864
|
Hi @FedericoTartarini , the PR is ready for review. Please take a look when you get a chance! |
| "hour": { | ||
| "name": "Hour", | ||
| "color": [ | ||
| ColNames.NONE: {ColNames.NAME: "None"}, |
There was a problem hiding this comment.
for the moment this is okay but it is very confusionary, please let me explain how we can fix this in the next meeting, basically we do not need both ColNames and this mapping dictionary, instead we should have the following:
class VariablesInfo
name: str
unit: str
.....
class ColNames:
# ==================== Time related column ====================
DAY = VariablesInfo(
name = "day of the Year"
unit="day"
col_name='day"
....)do not implement this now. I opened an issue so we can fix it next week.
FedericoTartarini
merged commit f54cde2
into
CenterForTheBuiltEnvironment:development
2 participants
This PR includes two main changes. These changes affect the same file(s) and are tightly coupled, so they are submitted together for consistency.
1. Standardize Global Column Name Usage
2. Refactored conversion logic by removing the conversion function mapping and implementing
convert_SI_to_IP().Summary by CodeRabbit
New Features
Improvements
Refactor